Add currency conversion support for BOLT 12 offers#3833
Add currency conversion support for BOLT 12 offers#3833shaavan wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
cc @jkczyz |
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Is this proposed change a response to a request from a specific user/users? |
|
Hi @joostjager! This PR is actually a continuation of the original thread that led to the The motivation behind it was to provide users with the ability to handle So, as a first step, we worked on refactoring most of the Offers-related code out of Hope that gives a clear picture of the intent behind this! Let me know if you have any thoughts or suggestions—would love to hear them. Thanks a lot! |
|
Another use case is Fedimint, where they'll want to include their own payment hash in the |
Does Fedimint plan to use the |
I believe with one. |
|
🔔 2nd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Removing @joostjager for now to stop bot spam. @shaavan and I have been working through some variations of this approach. |
vincenzopalazzo
left a comment
vincenzopalazzo
left a comment
There was a problem hiding this comment.
Concept ACK for me
I was just looking around to sync with this Offer Flow
shaavan
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3833 +/- ##
==========================================
+ Coverage 89.34% 89.37% +0.02%
==========================================
Files 180 180
Lines 138480 140045 +1565
Branches 138480 140045 +1565
==========================================
+ Hits 123730 125164 +1434
- Misses 12129 12295 +166
+ Partials 2621 2586 -35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hmm, yea, its an interesting question. Naively it seems like going the async direction makes sense - fetching currency conversion is likely something that happens via the internet so it should be async. But once we complete the event pipeline (presumably by bubbling these new events up as top-level On the flip side, doing currency conversion via a sync (cached) API doesn't seem so bad when you look at actual currency conversion APIs. Instead of being individual APIs that you hit to request the conversion from currency A to B, my glance through available options seems to mostly point to them being a single request that returns a JSON map of all the known currencies to their value in some fixed base currency (eg see https://openexchangerates.org/). Thus, in practice caching should be quite simple/a single up-front request. This does leave some complexity in ensuring you dont block the node on startup waiting for the conversion API response but presumably we want a fallible API anyway... |
|
Thanks for the thoughtful points, @TheBlueMatt On currency conversion specifically, I agree that a synchronous API is the better default. In practice it nudges users toward exactly the model they should be using anyway: cached, pre-fetched rates rather than just-in-time lookups. That said, I wanted to discuss one broader issue around Flow events in general.
I think this is a very fair criticism of the event-based model as it stands. Even if we move currency conversion to a synchronous, flow-owned API, we still seem to want some mechanism that lets a user “catch” an For example, @jkczyz mentioned a Fedimint use case that would require exactly this kind of interception and analysis. One idea we had explored was keeping a separate event queue inside So the one question left is this: How should we introduce Flow-events, or a Flow-event-like mechanism, that allows users to asynchronously inspect and reason about Would love to get your thoughts on this! |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
Oops sorry took me a minute to get back to this. Its a really good question I don't have a great answer for. In general in the This doesn't, of course, help us in
IMO we should generally default to the last option, avoiding adding any specific handling until we have a real specific use-case in mind and can see how to implement for that clearly. eg the fedi logic may well often run without a channelmanager or at least can punt the offers message handling out to a flow anyway. |
shaavan
force-pushed
the
currency
branch
2 times, most recently
from
c0bf606 to
1668749
Compare
|
Updated .10 → .11 Changes:
See the updated PR description for full details. |
Adds a `CurrencyConversion` trait allowing users to provide logic for converting currency-denominated amounts into millisatoshis. LDK itself cannot perform such conversions as exchange rates are external, time-dependent, and application-specific. Instead, the conversion logic must be supplied by the user. This trait forms the foundation for supporting Offers denominated in fiat currencies while keeping exchange-rate handling outside the core protocol logic.
shaavan
changed the title
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I think this looks good, yea.
| /// This convention ensures amounts remain precise and purely integer-based when parsing and | ||
| /// validating BOLT12 invoice requests. | ||
| pub trait CurrencyConversion { | ||
| /// Returns the conversion rate in **msats per minor unit** for the given |
There was a problem hiding this comment.
Is this actually enough precision? For USD its fine (result of ~14K these days), but there's some currencies where this is a much lower value (Lebanese Pound this is something like 16 currently, afaict). If LBP inflates a bit more its pretty easy to see this getting into the mid/low single digits, which would make this too low precision.
There was a problem hiding this comment.
I can think of a few possible approaches to address this:
-
Return
msats_per_major_unit.
This would be the simplest option. However, it doesn't fully solve the precision issue for very weak currencies such as LBP, where even the major unit may still map to a relatively small number of msats. -
Return
microsats_per_major_unit.
This increases the resolution while keeping the API relatively simple. It should provide sufficient precision for most currencies in the near future without significantly complicating the interface. -
Return a rational conversion rate, e.g.
(numerator: u128, denominator: u128), where:
1 CUR = (numerator / denominator) * 1 msat
This is the most flexible approach and would preserve precision even for extremely weak currencies. The trade-off is that it introduces a bit more complexity to the API.
Happy to hear thoughts on which direction would make the most sense here.
| /// This represents a user-level policy (e.g., allowance for exchange-rate | ||
| /// drift or cached data) and does not directly affect fiat-to-msat conversion | ||
| /// outside of range computation. | ||
| fn tolerance_percent(&self) -> u8; |
There was a problem hiding this comment.
This doesn't appear to be called in the current patchset? Also the docs aren't nearly clear enough on how this is (will be?) used.
There was a problem hiding this comment.
I kept tolerance_percent here mainly to discuss whether this is the right place to introduce the tolerance mechanism.
The idea is to account for possible exchange-rate drift between the payer and the payee by deriving a small acceptable range around the converted msat value. The intended usage would roughly be as follows.
If we are the payee and receive an InvoiceRequest for a fiat-denominated offer:
If the amount is specified in the InvoiceRequest, we accept it as long as:
invoice_request_amount ≥ offer.resolved_amount() - tolerance
If the amount is not specified in the InvoiceRequest, we simply set:
invoice_amount = offer.resolved_amount()
If we are the payer, we can set:
invoice_request_amount = offer.resolved_amount() + tolerance
This gives the payee some headroom when constructing the invoice, ensuring the payment does not fail if the exchange rate shifts slightly between the time the InvoiceRequest is created and when the invoice is issued.
| InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(&invoice_request.inner)?; | ||
| let amount_msats = InvoiceBuilder::<DerivedSigningPubkey>::amount_msats( | ||
| &invoice_request.inner, | ||
| conversion, |
There was a problem hiding this comment.
Do we want to allow for a more flexible API in the flow itself? eg leaving amount_msats alone and having the logic here do the currency conversion by hand instead so that someone could use an OffersMessageFlow with their own currency conversion logic?
Makes OffersMessageFlow generic over a CurrencyConversion implementation, propagating the parameter through to ChannelManager. Upcoming changes will introduce currency conversion support in BOLT 12 message handling, which requires access to conversion logic from both ChannelManager and OffersMessageFlow. By threading the conversion abstraction through OffersMessageFlow now, subsequent commits can use it directly without introducing temporary plumbing or refactoring the type hierarchy later.
Extends `OfferBuilder` to allow creating Offers whose amount is denominated in a fiat currency instead of millisatoshis. To ensure such Offers can later be processed correctly, currency amounts may only be set when the caller provides a `CurrencyConversion` implementation capable of resolving the amount into millisatoshis. Since amount correctness checks are now performed directly in the amount setters, they can be removed from the `build()` method. This introduces the first layer of currency support in Offers, allowing them to be created with currency-denominated amounts.
To support currency-denominated Offers, the InvoiceRequest builder needs to resolve the Offer amount at multiple points during construction. This occurs when explicitly setting `amount_msats` and again when the InvoiceRequest is finalized via `build()`. To avoid repeatedly passing a `CurrencyConversion` implementation into these checks, the builder now stores a reference to it at creation time. This allows the builder to resolve currency-denominated Offer amounts whenever validation requires it. As part of this change, `InvoiceRequest::amount_msats()` is updated to use the provided `CurrencyConversion` to resolve the underlying Offer amount when necessary.
Adds currency conversion support when responding to an `InvoiceRequest` and constructing the `InvoiceBuilder`. When the underlying Offer specifies its amount in a currency denomination, the `CurrencyConversion` implementation is used to resolve the payable amount into millisatoshis and ensure the invoice amount satisfies the Offer's requirements. This reintroduces the currency validation intentionally skipped during `InvoiceRequest` parsing, keeping parsing focused on structural validation while enforcing amount correctness at the time the Invoice is constructed.
Adds tests covering Offers whose amounts are denominated in fiat currencies. These tests verify that: * currency-denominated Offer amounts can be created * InvoiceRequests correctly resolve amounts using CurrencyConversion * Invoice construction validates and enforces the payable amount This ensures the full Offer → InvoiceRequest → Invoice flow works correctly when the original Offer amount is specified in currency. Co-authored By: CodeX
|
Updated .11 → .12 Changes:
|
7 participants
This PR adds support for currency-denominated Offers in LDK’s BOLT 12 offer-handling flow.
Previously, Offers could only specify their amount in millisatoshis. However, BOLT 12 allows Offers to be denominated in other currencies such as fiat. Supporting this requires converting those currency amounts into millisatoshis at runtime when validating payments and constructing invoices.
Because exchange rates are external, time-dependent, and application-specific, LDK cannot perform these conversions itself. Instead, this PR introduces a
CurrencyConversiontrait which allows applications to provide their own logic for resolving currency-denominated amounts into millisatoshis. LDK remains exchange-rate agnostic and simply invokes this trait whenever a currency amount must be resolved.To make this conversion logic available throughout the BOLT 12 flow,
OffersMessageFlowis parameterized over aCurrencyConversionimplementation and the abstraction is threaded through the offer handling pipeline.With this in place:
OfferBuildercan now create Offers whose amounts are denominated in currencies instead of millisatoshis•
InvoiceRequesthandling can resolve Offer amounts when validating requests•
InvoiceBuilderenforces that the final invoice amount satisfies the Offer’s requirements after resolving any currency denominationCurrency validation is intentionally deferred until invoice construction when necessary, keeping earlier stages focused on structural validation while ensuring the final payable amount is correct.
Tests are added to cover the complete Offer → InvoiceRequest → Invoice flow when the original Offer amount is specified in a currency.